Added calculation of the distance between a line and a point #40
Conversation
|
@Darya177777 : CI tests failed. Make sure your fork is sync'ed with this repo. You might need to just update the line numbers in some expected output files? |
doc/operators.sgm
Outdated
| @@ -331,7 +331,7 @@ | |||
| a non-boolean operator returning the distance between two | |||
| objects in radians. Currently, | |||
| <application>pgSphere</application> supports only distances | |||
| between points, circles, and between point and circle. If the | |||
| between points, circles, between point and circle, and between point and line. If the | |||
There was a problem hiding this comment.
Does this line exceed 79 characters? If so, please word-wrap.
pgs_line.sql.in
Outdated
| IMMUTABLE STRICT; | ||
|
|
||
| COMMENT ON FUNCTION dist(sline, spoint) IS | ||
| 'distance between spherical line and spherical point'; |
There was a problem hiding this comment.
All the other COMMENT ON FUNCTIONs in this file start with "returns..." I know that seems redundant, but I think it's best to fit in with the convention of the other COMMENTs.
expected/line.out
Outdated
| @@ -134,4 +134,34 @@ SELECT sline ( spoint '(0, 0d)', spoint '(0.000001d, 0d)' ) # | |||
| f | |||
| (1 row) | |||
|
|
|||
|
|
|||
| -- checking the distance between a line and a point | |||
| SELECT round( CAST(sline '( 90d, 0d, 0d, XYZ ), 40d ' <-> spoint '( 0d, 90d )' as numeric), 14); | |||
There was a problem hiding this comment.
What about the distance between a point and a line? I.e., the commutative property. Is that implemented? If not, it should be. If it is, then it should be tested.
SELECT round( CAST(spoint '( 0d, 90d )' <-> sline '( 90d, 0d, 0d, XYZ ), 40d ' as numeric), 14);
|
All fixed. |
OK, looking better! @vitcpp will probably have some comments as well. |
|
@Darya177777 Could you also please rebase on the newest version and update the upgrade script upgrade_scripts/pg_sphere--1.2.3--1.3.0.sql.in. Such script is used when updating the extension to a newer version using ALTER EXTENSION UPDATE TO command. Once you have introduced a new function the upgrade script should be updated. |
src/line.c
Outdated
|
|
||
| vector3d_spoint(&fp, &v_beg); | ||
| vector3d_spoint(&sp, &v_end); | ||
| if (FPle(spoint_dist(p, &fp), spoint_dist(p, &sp))) |
There was a problem hiding this comment.
I would propose to this code faster like this:
fpdist = spoint_dist(p, &fp);
spdist = spoint_dist(p, &sp);
return Min(fpdist, spdist);
src/line.c
Outdated
| Datum | ||
| sphereline_point_distance(PG_FUNCTION_ARGS) | ||
| { | ||
| SLine *s = (SLine *) PG_GETARG_POINTER(0); |
There was a problem hiding this comment.
I would propose to add const prefix in local variable declarations.
src/line.c
Outdated
| Datum | ||
| sphereline_point_distance_com(PG_FUNCTION_ARGS) | ||
| { | ||
| SPoint *p = (SPoint *) PG_GETARG_POINTER(0); |
There was a problem hiding this comment.
I would propose to add const prefix in local variable declarations.
expected/line.out
Outdated
| 0.87266462599717 | ||
| (1 row) | ||
|
|
||
| SELECT round( CAST(sline '( 90d, 0d, 0d, XYZ ), 40d ' <-> spoint '( 0d, 90d )' as numeric), 14) = |
There was a problem hiding this comment.
I think round(CAST... is not necessary.
I would propose to add SET extra_float_digits TO -3; in the beginning of file. It reduces the number of output digits.
I've attached the modified line.sql
line.sql.zip
|
@Darya177777 Please, ignore this comment. It seems I was wrong when claimed these cases to be wrong.
|
|
@Darya177777 Could you please squash two commits into one. |
doc/index.html
Outdated
| @@ -0,0 +1,227 @@ | |||
| <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN""http://www.w3.org/TR/html4/loose.dtd"> | |||
There was a problem hiding this comment.
This file is generated. It should not be added in the commit. Could you please remove it?
doc/types.html
Outdated
| @@ -0,0 +1,272 @@ | |||
| <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN""http://www.w3.org/TR/html4/loose.dtd"> | |||
There was a problem hiding this comment.
This file should be removed from the commit.
doc/x20.html
Outdated
| @@ -0,0 +1,243 @@ | |||
| <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN""http://www.w3.org/TR/html4/loose.dtd"> | |||
There was a problem hiding this comment.
Remove this file from the commit.
doc/x46.html
Outdated
| @@ -0,0 +1,423 @@ | |||
| <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN""http://www.w3.org/TR/html4/loose.dtd"> | |||
There was a problem hiding this comment.
Remove this file from the commit.
expected/init_test.out
Outdated
| @@ -0,0 +1,37 @@ | |||
| SET client_min_messages TO NOTICE; | |||
There was a problem hiding this comment.
Remove this file from the commit.
log/initdb.log
Outdated
| @@ -0,0 +1,37 @@ | |||
| Running in no-clean mode. Mistakes will not be cleaned up. | |||
There was a problem hiding this comment.
Remove this file from the commit.
log/postmaster.log
Outdated
| @@ -0,0 +1,376 @@ | |||
| 2023-08-08 15:16:01.222 +07 postmaster[14615] LOG: starting PostgreSQL 15.3 on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0, 64-bit | |||
There was a problem hiding this comment.
Remove this file from the commit.
logfile
Outdated
| @@ -0,0 +1,749 @@ | |||
| 2023-08-08 14:58:03.794 +07 [12364] LOG: starting PostgreSQL 15.3 on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0, 64-bit | |||
There was a problem hiding this comment.
Remove this file from the commit.
Darya177777
force-pushed
the
master
branch
2 times, most recently
from
f44e926 to
f62c70f
Compare
|
PR seems to be ok. I need to do some testing before merge. |
@vitcpp, doesn't GitHub automatically squash commits on merge these days? I've been under the impression that it does, but maybe I'm used to GitLab. |
|
@esabol I'm not sure about github feature to squash commits. I will check it. I'm concerned which final comment will appear in commit after squashing. I'm not sure that simple joining of commit messages is a good idea. Furthermore, it is harder to review multiple commits where latest commit overwrites some changes of the previous commit. PR should contain the final version of the changes. It can contain multiple commits but these commits should be used to logically separate the changes. They should not touch the same code. |
|
@Darya177777 Could you please fix the tests? Thank you. |
src/sscan.c
Outdated
| @@ -1,6 +1,6 @@ | |||
| #line 2 "sscan.c" | |||
There was a problem hiding this comment.
This file is generated. It should be removed from the commit.
Darya177777
force-pushed
the
master
branch
2 times, most recently
from
d201938 to
337928e
Compare
|
@Darya177777 You have added sscan.c file to the commit. It shouldn't be added. Could you please remove it from the commit. |
|
@vitcpp , All fixed |
@vitcpp, you can edit the merge commit message to whatever you want when squashing and merging, according to this: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/incorporating-changes-from-a-pull-request/about-pull-request-merges#squash-and-merge-your-commits |
@esabol Thank you very much! I will take a look. |
There was a problem hiding this comment.
@Darya177777 It seems we missed to make some changes in pg_sphere--1.2.3--1.3.0.sql.in. This script is used to upgrate pgsphere versions in the existing database. Just copy the contents of pgs_line.sql.in into this file.
|
@vitcpp I have made some changes in pg_sphere--1.2.3--1.3.0.sql.in. Please, re-review. |
No description provided.